Skip to content

[CORRUPTED] Synthetic Benchmark PR #102472 - feat: Consolidate adjacent traces & move next to search #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tomerqodo wants to merge 5 commits into
base: base_pr_102472_20251204_9590
Choose a base branch
from
Open

[CORRUPTED] Synthetic Benchmark PR #102472 - feat: Consolidate adjacent traces & move next to search #14

tomerqodo wants to merge 5 commits into from

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025 •
edited by qodo-code-review bot
Loading

User description

Benchmark PR getsentry#102472

Type: Corrupted (contains bugs)

Original PR Title: feat: Consolidate adjacent traces & move next to search
Original PR Description: ## Description

This is a follow up to getsentry#96510. The new approach is that the buttons are close to the other interaction buttons, so the user doesn't have to go and find this at the very bottom of the page. Since the search and the "Open in Explore" are already trace related, the user doesn't have to switch "context" and the buttons wouldn't need any additional text in it, except the tooltip if users are curious what it is.

Significant changes

  • useFindPreviousTrace and useFindNextTrace have been consolidated into useFindAdjacentTrace, which reduces code duplication
  • The tooltip has been moved to the outer most ButtonBar, since it wouldn't need a tooltip for each button
  • A own component has been created for the Buttons so it can be easily moved around

📹

The UI reacts a little slow in the GIF since my computer was a little low on resources at this point in time

Kapture 2025-10-31 at 13 22 55

Original PR URL: getsentry#102472

PR Type

Enhancement

Description

  • Consolidate useFindNextTrace and useFindPreviousTrace into unified useFindAdjacentTrace hook

  • Simplify trace navigation button component using LinkButton from scraps library

  • Extract trace links navigation into dedicated TraceLinksNavigation component with shared tooltip

  • Move trace navigation buttons from bottom of page to toolbar near search and explore buttons

Diagram Walkthrough

flowchart LR
  A["useFindNextTrace<br/>useFindPreviousTrace"] -->|consolidate| B["useFindAdjacentTrace"]
  C["TraceLinkNavigationButton<br/>with individual tooltips"] -->|simplify| D["TraceLinkNavigationButton<br/>with LinkButton"]
  E["Bottom of page<br/>TraceLinksNavigationContainer"] -->|move| F["TraceToolbar<br/>TraceLinksNavigation"]
  B -->|used by| D
  D -->|wrapped in| F
Loading

File Walkthrough
Relevant files
Enhancement
useFindLinkedTraces.ts
Consolidate adjacent trace finding logic                                 

static/app/views/performance/newTraceDetails/traceLinksNavigation/useFindLinkedTraces.ts

  • Consolidate two separate hooks (useFindNextTrace and
    useFindPreviousTrace) into single useFindAdjacentTrace hook
  • Rename parameters to use adjacentTrace prefix instead of
    nextTrace/previousTrace for consistency
  • Unify query logic with direction-based conditional for search query
    and enabled conditions
  • Simplify return type by removing sampled field and using consistent
    structure for both directions
  • Add useMemo wrapper for return value to optimize performance
 +81/-120
traceLinkNavigationButton.tsx
Simplify button component with LinkButton                               

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx

  • Replace custom styled components with LinkButton from
    @sentry/scraps/button library
  • Remove individual tooltips and styling logic from button component
  • Simplify component to use single useFindAdjacentTrace hook instead of
    two separate hooks
  • Consolidate direction-specific logic using useMemo for timestamp and
    icon direction
  • Remove placeholder component and conditional rendering logic
  • Reduce component from ~120 lines to ~30 lines
 +48/-133
traceLinksNavigation.tsx
New component for trace links navigation                                 

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinksNavigation.tsx

  • New component that wraps trace navigation buttons with shared tooltip
    and button bar
  • Handles feature flag check and source validation for showing linked
    traces
  • Consolidates tooltip logic previously duplicated in individual buttons
  • Uses ButtonBar from scraps library for consistent button grouping
  • Manages rendering of both previous and next trace buttons together
 +65/-0   
traceWaterfall.tsx
Relocate trace navigation to toolbar                                         

static/app/views/performance/newTraceDetails/traceWaterfall.tsx

  • Move trace navigation buttons from bottom of page to toolbar near
    search and explore buttons
  • Replace TraceLinksNavigationContainer with TraceLinksNavigation
    component in toolbar
  • Remove feature flag and conditional rendering logic from waterfall
    component
  • Remove unused imports and styled components
  • Simplify component by delegating trace links logic to dedicated
    component
 +5/-47   

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing auditing: The new hooks and navigation actions perform trace lookups and navigation without adding
any audit logging for these potentially sensitive user actions.

Referred Code 
const searchQuery =
  direction === 'next'
    ? `sentry.previous_trace:${currentTraceId}-${currentSpanId}-1`
    : `id:${adjacentTraceSpanId} trace:${adjacentTraceId}`;

const enabled =
  direction === 'next'
    ? !!currentTraceId && !!currentSpanId
    : hasAdjacentTraceLink &&
      adjacentTraceSampled &&
      !!adjacentTraceSpanId &&
      !!adjacentTraceId;

const {data, isError, isPending} = useSpans(
  {
    search: searchQuery,
    fields: ['id', 'trace'],
    limit: 1,
    enabled,
    projectIds: projectId ? [projectId] : [],
    pageFilters: {


 ... (clipped 18 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Error handling gaps: The hook relies on isError/isPending but does not surface or log contextual errors or
handle malformed adjacent trace attributes beyond disabling queries, which may hinder
debugging.

Referred Code 
const {data, isError, isPending} = useSpans(
  {
    search: searchQuery,
    fields: ['id', 'trace'],
    limit: 1,
    enabled,
    projectIds: projectId ? [projectId] : [],
    pageFilters: {
      environments: environment ? [environment] : [],
      projects: projectId ? [projectId] : [],
      datetime: {
        start: adjacentTraceStartTimestamp
          ? new Date(adjacentTraceStartTimestamp * 1000).toISOString()
          : '',
        end: adjacentTraceEndTimestamp
          ? new Date(adjacentTraceEndTimestamp * 1000).toISOString()
          : '',
        period: null,
        utc: true,
      },
    },


 ... (clipped 35 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Query input validation: The search query is built from attributes (trace/span IDs) without explicit
validation/sanitization, which could risk malformed queries if upstream data is
unexpected.

Referred Code 
const searchQuery =
  direction === 'next'
    ? `sentry.previous_trace:${currentTraceId}-${currentSpanId}-1`
    : `id:${adjacentTraceSpanId} trace:${adjacentTraceId}`;

const enabled =
  direction === 'next'
    ? !!currentTraceId && !!currentSpanId
    : hasAdjacentTraceLink &&
      adjacentTraceSampled &&
      !!adjacentTraceSpanId &&
      !!adjacentTraceId;

const {data, isError, isPending} = useSpans(
  {
    search: searchQuery,
    fields: ['id', 'trace'],
    limit: 1,
    enabled,
    projectIds: projectId ? [projectId] : [],
    pageFilters: {


 ... (clipped 6 lines)

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
The trace navigation logic is broken

The trace navigation feature is broken due to two bugs: the "previous" and
"next" button icons are swapped, and the navigation URL uses the current trace's
timestamp instead of the adjacent trace's, causing incorrect view centering.

Examples:

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx [45-58] 
    if (direction === 'previous') {
      return {
        adjacentTraceEndTimestamp: currentTraceStartTimestamp,
        adjacentTraceStartTimestamp: linkedTraceWindowTimestamp,
        iconDirection: 'right' as const,
        ariaLabel: t('Previous Trace'),
      };
    }
    return {
      adjacentTraceEndTimestamp: linkedTraceWindowTimestamp,

 ... (clipped 4 lines)
static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx [94-101] 
      to={getTraceDetailsUrl({
        traceSlug: traceId ?? '',
        spanId: traceSpanId,
        dateSelection,
        timestamp: currentTraceStartTimestamp,
        location,
        organization,
      })}

Solution Walkthrough:

Before:

// file: static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx

const { iconDirection } = useMemo(() => {
  if (direction === 'previous') {
    return {
      // ...
      iconDirection: 'right' as const, // Incorrect: should be 'left'
    };
  }
  return {
    // ...
    iconDirection: 'left' as const, // Incorrect: should be 'right'
  };
}, ...);

return (
  <LinkButton
    // ...
    to={getTraceDetailsUrl({
      // ...
      timestamp: currentTraceStartTimestamp, // Incorrect: uses current trace's timestamp
    })}
  />
);

After:

// file: static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx

const { iconDirection } = useMemo(() => {
  if (direction === 'previous') {
    return {
      // ...
      iconDirection: 'left' as const, // Corrected
    };
  }
  return {
    // ...
    iconDirection: 'right' as const, // Corrected
  };
}, ...);

return (
  <LinkButton
    // ...
    to={getTraceDetailsUrl({
      // ...
      timestamp: linkedTraceWindowTimestamp, // Corrected: uses calculated timestamp for adjacent trace
    })}
  />
);
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies two critical bugs in the trace navigation logic—swapped icons and an incorrect timestamp—that break the feature's functionality and user experience.

High
Possible issue
Correct chevron icon direction

Change the iconDirection for the 'previous' trace button from 'right' to 'left'
to correctly indicate backward navigation.

static/app/views/performance/newTraceDetails/traceLinksNavigation/traceLinkNavigationButton.tsx [49]

-iconDirection: 'right' as const,
+iconDirection: 'left' as const,
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a UI bug where the navigation icon for the 'previous' trace button points in the wrong direction, which would confuse users.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tomerqodo @Lms24 @JPeer264